feat(input): Implement SDL3 input and window management#2639
feat(input): Implement SDL3 input and window management#2639githubawn wants to merge 24 commits into
Conversation
a785545 to
7cc0861
Compare
|
| Filename | Overview |
|---|---|
| Core/GameEngineDevice/Source/SDL3Device/GameClient/SDL3Input.cpp | Core implementation of SDL3Mouse, SDL3Keyboard, and SDL3InputManager; contains the previously-flagged scancode truncation, frame-count bounds issue, and ring-buffer event dispatch logic |
| Core/GameEngineDevice/Source/SDL3GameEngine.cpp | SDL3GameEngine init/update/event-loop; headless path now correctly falls through to GameEngine::init(); text-input forwarding with hand-rolled UTF-8 decoder looks correct |
| Core/GameEngineDevice/Source/SDL3Device/GameClient/SDL3Cursor.cpp | ANI cursor loader via SDL3_image; correctly patches RIFF cbSize for Generals' non-standard format; static cursor resource array uses proper bounds checks |
| GeneralsMD/Code/Main/WinMain.cpp | SDL3 window creation, splash rendering, and engine factory wired under RTS_SDL3_ENABLE; two NULL usages in new SDL3 code should be nullptr per project style |
| Core/GameEngineDevice/Include/SDL3Device/GameClient/SDL3Input.h | Class declarations for SDL3Mouse/Keyboard/InputManager; translateScanCodeToKeyVal still declares parameter as unsigned char (previously flagged scancode truncation issue) |
| cmake/sdl3.cmake | vcpkg-first, FetchContent fallback strategy for SDL3 and SDL3_image; static library aliases and Windows system library linkage applied correctly |
| GeneralsMD/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DGameClient.h | Mouse/keyboard factory methods correctly conditionalized between SDL3 and Win32/DirectInput paths using #if RTS_SDL3_ENABLE |
Sequence Diagram
sequenceDiagram
participant WinMain
participant SDL3GameEngine
participant SDL3InputManager
participant SDL3Mouse
participant SDL3Keyboard
participant SDL3CursorManager
WinMain->>SDL3GameEngine: CreateGameEngine()
WinMain->>WinMain: SDL_Init() + SDL_CreateWindow()
WinMain-->>SDL3GameEngine: TheSDL3Window set
SDL3GameEngine->>SDL3GameEngine: init()
SDL3GameEngine->>SDL3InputManager: new SDL3InputManager(window)
SDL3InputManager->>SDL3InputManager: openFirstGamepad()
loop Game Loop
SDL3GameEngine->>SDL3InputManager: update()
SDL3InputManager->>SDL3InputManager: SDL_PollEvent()
SDL3InputManager->>SDL3InputManager: processGamepadInput()
SDL3InputManager->>SDL3Mouse: addSDLEvent() [mouse events]
SDL3InputManager->>SDL3Keyboard: addSDLEvent() [key events]
SDL3Mouse->>SDL3InputManager: getNextMouseEvent()
SDL3Mouse->>SDL3Mouse: translateEvent() + scaleMouseCoordinates()
SDL3Keyboard->>SDL3InputManager: getNextKeyboardEvent()
SDL3Keyboard->>SDL3Keyboard: translateScanCodeToKeyVal()
end
SDL3Mouse->>SDL3CursorManager: initResources() / getCursor()
SDL3CursorManager->>SDL3CursorManager: loadANI() via SDL3_image
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
GeneralsMD/Code/Main/WinMain.cpp:924
Two `NULL` literals in the new SDL3 code should be `nullptr` to match the project's C++ style rule. Both are passed to SDL3 API parameters that accept pointer types (`void*` default value and `const SDL_Rect*` source rect), so `nullptr` is the correct C++ null pointer constant here.
```suggestion
ApplicationHWnd = (HWND)SDL_GetPointerProperty(props, SDL_PROP_WINDOW_WIN32_HWND_POINTER, nullptr);
```
### Issue 2 of 3
GeneralsMD/Code/Main/WinMain.cpp:948
`NULL` for the source `SDL_Rect*` parameter should be `nullptr` per the project's C++ style rule — the same pattern as line 924. Both are pointer-typed SDL3 API parameters, not integer handles.
### Issue 3 of 3
Core/GameEngineDevice/Source/SDL3Device/GameClient/SDL3Input.cpp:892-899
The `button` parameter is declared but never read inside `handleGamepadButton` — it is always ignored. All call-sites that map analogue stick directions already pass `SDL_GAMEPAD_BUTTON_INVALID` as a placeholder. Removing the dead parameter eliminates a compiler warning and makes the intent explicit.
```suggestion
void SDL3InputManager::handleGamepadButton(bool& currentState, bool isDown, std::function<void(bool)> action)
{
if (isDown != currentState)
{
action(isDown);
currentState = isDown;
}
}
```
Reviews (25): Last reviewed commit: "cleaned up comments" | Re-trigger Greptile
| OVERRIDE_FIND_PACKAGE | ||
| ) | ||
|
|
||
| FetchContent_Declare( |
There was a problem hiding this comment.
Do you think we need SDL3_image? Looks like it's used to parse .ico for animated cursors? Maybe there is already code for that we can adapt
There was a problem hiding this comment.
directinput lets windows directly handle the cursor rendering so new code and SDL3_image was needed
There was a problem hiding this comment.
In that case consider putting AnimatedCursor in its own file.
There was a problem hiding this comment.
SDL3_image seem overkill for handling animated cursors though as @stephanmeesters says, its just been used to decode the ico format, SDL itself already provides support for the animated cursors once the files are decoded and loaded.
There was a problem hiding this comment.
Moved AnimatedCursor into its own file and switched to the SDL_CreateAnimatedCursor API.
Regarding the dependency: this is a conscious choice. While SDL core handles the animation logic, it doesn't decode ANI or ICO files. I’d rather offload that to SDL_image than maintain bespoke binary parsing. Using the library also makes it trivial for modders to use modern formats in the future.
I think this is the most maintainable path. Does that sound reasonable?
There was a problem hiding this comment.
Regarding the dependency: this is a conscious choice. While SDL core handles the animation logic, it doesn't decode ANI or ICO files. I’d rather offload that to SDL_image than maintain bespoke binary parsing. Using the library also makes it trivial for modders to use modern formats in the future.
I think this is the most maintainable path. Does that sound reasonable?
Also @bobtista ’s #1785 could then use it for jpg and png encoding
| @@ -9,6 +9,16 @@ option(RTS_BUILD_OPTION_ASAN "Build code with Address Sanitizer." OFF) | |||
| option(RTS_BUILD_OPTION_VC6_FULL_DEBUG "Build VC6 with full debug info." OFF) | |||
| option(RTS_BUILD_OPTION_FFMPEG "Enable FFmpeg support" OFF) | |||
|
|
|||
| # Enable SDL3 by default for modern builds | |||
| if(NOT IS_VS6_BUILD) | |||
| option(SAGE_USE_SDL3 "Enable SDL3 input/window backend" ON) | |||
There was a problem hiding this comment.
Options should be on top of the file and be something like RTS_BUILD_OPTION_SDL3
|
|
||
| namespace { | ||
|
|
||
| Bool DecodeNextUtf8Codepoint(const char* text, size_t length, size_t& offset, UnsignedInt& outCodepoint) |
There was a problem hiding this comment.
Do we need this, maybe better placed in UnicodeString? Or some other helper if it's generic stuff
There was a problem hiding this comment.
Good point.
I looked at #2045 and #2528 which are working on UTF-8 infrastructure.
If one of those lands first, forwardTextInputEvent could use the bulk Utf8_To_Utf16Le conversion and iterate the resulting wchar_t string directly, making this function unnecessary.
Happy to refactor once there's a clear winner between those two. For now it's self-contained here.
e365605 to
627ef23
Compare
Consolidated all work from the test/sdl3-backport branch into a single atomic commit: - Centralized input management via SDL3InputManager. - Hardened Ani/RIFF cursor loading with robust bounds checking. - Native Gamepad support with analogue stick-to-mouse emulation and custom RTS mappings. - Modernized focus and capture handling for better stability during Alt-Tab. - Standardized and secured string operations throughout the SDL3 path.
Consolidated all work from the test/sdl3-backport branch into a single atomic commit: - Centralized input management via SDL3InputManager. - Hardened Ani/RIFF cursor loading with robust bounds checking. - Native Gamepad support with analogue stick-to-mouse emulation and custom RTS mappings. - Modernized focus and capture handling for better stability during Alt-Tab. - Standardized and secured string operations throughout the SDL3 path. - Updated credit attribution (fbraz3).
eb9908a to
534e694
Compare
| } | ||
| } | ||
|
|
||
| AnimatedCursor* SDL3CursorManager::loadANI(const char* filepath) |
There was a problem hiding this comment.
libsdl-org/SDL_image#730
Ran into a bug in SDL_image. Ignore SDL3CursorManager::loadANI if you are reviewing this.
There was a problem hiding this comment.
Ran into a second issue after the first one was fixed. Updated the code, but there is still one hack left.
xezon
left a comment
There was a problem hiding this comment.
This needs polishing. Not yet reviewed extensively until the obvious style issues are fixed.
| "git-tree": "3f05e04b9aededb96786a911a16193cdb711f0c9" | ||
| } | ||
| ] | ||
| "version": 1, |
| include(cmake/gamespy.cmake) | ||
| include(cmake/lzhl.cmake) | ||
|
|
||
| if(SAGE_USE_SDL3 AND NOT IS_VS6_BUILD) |
There was a problem hiding this comment.
Where does the SAGE_ prefix come from? We have not used that before for anything else.
|
|
||
| #if SAGE_USE_SDL3 | ||
| #include <SDL3/SDL.h> | ||
| #include "SDL3GameEngine.h" |
There was a problem hiding this comment.
This SDL code is added to a file called WinMain (for Windows). Is this intentional?
There was a problem hiding this comment.
This was intentional. The SDL3 input backend is designed with a future splitscreen skirmish feature in mind, supporting multiple simultaneous mice/keyboards so that 2–8 players can share a single machine rather than needing separate computers, either controlling the same or unique armies.
SDL3 input doesn't function without an SDL window, so creating the window here was the minimum viable approach to make the input backend functional. The Win32/DirectInput path doesn't lend itself to the multi-device model SDL3 enables, which is why SDL3 is the foundation for this direction.
There was a problem hiding this comment.
Shouldn't we have a SDL3Main then? I have seen other forks do that.
Maybe make WinMain and SDL3Main share code if they do. But basically get rid of the ifdef approach.
|
|
||
| ParticleSystemManager* SDL3GameEngine::createParticleSystemManager(Bool dummy) | ||
| { | ||
| (void)dummy; |
|
|
||
| namespace { | ||
|
|
||
| Bool DecodeNextUtf8Codepoint(const char* text, size_t length, size_t& offset, UnsignedInt& outCodepoint) |
There was a problem hiding this comment.
This function looks out of place. Bobtista was also working on Utf8 decoding in another change. It would be good to have this as a utility somewhere accessible engine wide, and not specific to this file.
| m_gamepad(nullptr), | ||
| m_precisionMode(FALSE), | ||
| m_lastUpdateTime(0), | ||
| m_isQuitting(FALSE) |
| handleGamepadButton(SDL_GAMEPAD_BUTTON_DPAD_RIGHT, m_state.buttonState[SDL_GAMEPAD_BUTTON_DPAD_RIGHT], SDL_GetGamepadButton(m_gamepad, SDL_GAMEPAD_BUTTON_DPAD_RIGHT), [&](bool d){ virtualPulseKey(SDL_SCANCODE_3, d); }); | ||
| handleGamepadButton(SDL_GAMEPAD_BUTTON_DPAD_DOWN, m_state.buttonState[SDL_GAMEPAD_BUTTON_DPAD_DOWN], SDL_GetGamepadButton(m_gamepad, SDL_GAMEPAD_BUTTON_DPAD_DOWN), [&](bool d){ virtualPulseKey(SDL_SCANCODE_4, d); }); | ||
| handleGamepadButton(SDL_GAMEPAD_BUTTON_LEFT_STICK, m_state.buttonState[SDL_GAMEPAD_BUTTON_LEFT_STICK], SDL_GetGamepadButton(m_gamepad, SDL_GAMEPAD_BUTTON_LEFT_STICK), [&](bool d){ if (d) TheMessageStream->appendMessage(GameMessage::MSG_META_SELECT_NEXT_IDLE_WORKER); }); | ||
| handleGamepadButton(SDL_GAMEPAD_BUTTON_RIGHT_STICK, m_state.buttonState[SDL_GAMEPAD_BUTTON_RIGHT_STICK], SDL_GetGamepadButton(m_gamepad, SDL_GAMEPAD_BUTTON_RIGHT_STICK), [&](bool d){ if (d) TheMessageStream->appendMessage(GameMessage::MSG_META_VIEW_COMMAND_CENTER); }); |
|
|
||
| # Enable SDL3 by default for modern builds | ||
| if(NOT IS_VS6_BUILD) | ||
| option(SAGE_USE_SDL3 "Enable SDL3 input/window backend" ON) |
There was a problem hiding this comment.
Please do not use SAGE_ keywords. We use RTS_
Maybe do RTS_SDL3_ENABLE. That is consistent style with RTS_CRASHDUMP_ENABLE
|
|
||
| # Enable SDL3 by default for modern builds | ||
| if(NOT IS_VS6_BUILD) | ||
| option(SAGE_USE_SDL3 "Enable SDL3 input/window backend" ON) |
There was a problem hiding this comment.
Why is this default enabled on non VS6 builds but Windows builds? Will SDL3 perform better than Win API?
There was a problem hiding this comment.
If we don't use DirectStorage and Raytracing the SDL3 boilerplate performs better than our current implementation even on Windows in trade for a slightly larger binary. Legacy version doesn't compile statistically against SDL3 and so VS6 changes are avoided.
|
Perhaps also check Fighter19's fork for SDL related implementations. As far as I am aware he has it all done. |
|
I have looked at all existing forks and attributed where possible. |
SDL3 Input Backend
This PR implements an SDL3-based input and windowing backend as a modern alternative to DirectInput and Win32 window creation. By utilizing SDL3, we bypass DirectInput emulation layers on modern systems, providing a lower-latency pipeline for Windows 11 and Wine/Linux users.
Input handling
Unified event manager that centralizes keyboard, mouse, and gamepad events into a thread-safe buffer
Animated cursor support via .ani (RIFF) file loading.
Gamepad Support (v0.1)
This is an initial baseline implementation focused on providing functional out-of-the-box playability. Feedback is encouraged regarding the ergonomics and logic of these default mappings.
Scope Note: This implementation provides a hardcoded default layout to establish core functionality. Advanced features, such as input remapping, radial menus, adjustable deadzones, are currently out of scope for this PR and may be addressed in future iterations.
The foundation of this backend was built using the SDL3 input work from the generalsX fork by fbraz3.
Todo: replicate to generals